Conversation
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
|
""" WalkthroughThis change updates GitHub Actions workflows to replace the use of removed Makefile targets ( Changes
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
14686-14688: Consider making the debug switch runtime-configurableHard-coding
PrintQueryPlans: falsekeeps CI logs clean, but it also makes ad-hoc troubleshooting harder because re-enabling requires modifying and re-committing test code. Allowing an environment variable override (defaulting tofalse) provides flexibility without polluting logs in normal runs.- Debug: plan.DebugConfiguration{ - PrintQueryPlans: false, - }, + Debug: plan.DebugConfiguration{ + PrintQueryPlans: os.Getenv("WG_PRINT_QUERY_PLANS") == "1", + },You’d just need to
import "os"at the top of the file (or in the existing import block).
Developers could then runWG_PRINT_QUERY_PLANS=1 go test ./...locally for deep inspection while CI remains silent.v2/Makefile (1)
11-13:test-quickmight silently miss race conditions & long-running testsQuick tests intentionally trade depth for speed, but two tweaks would give you broader safety at virtually no cost:
-go test -count=1 -run=. -bench=. -benchtime=1x ./... +go test -race -short -count=1 -run=. -bench=. -benchtime=1x ./...•
-racekeeps the data-race net in place (only ~1.3-1.5× slower for most packages).
•-shortlets packages respecttesting.Short()and skip truly heavy cases, so overall wall-time stays low.If you do adopt
-race, remember to gate it in the workflow (skip on Windows) the same way the full test does..github/workflows/execution.yml (1)
38-40: Step label no longer reflects the actionNow that the command is
make test-quick, consider renaming the step to “Quick tests & benchmarks” to keep workflow logs self-describing:- - name: CI + - name: Quick tests & benchmarks working-directory: execution - run: make test-quick + run: make test-quickPurely cosmetic but helps future triage.
.github/workflows/v2.yml (1)
41-44: Align step name with the newmake test-quickcommandSame naming nitpick as in the execution workflow to keep logs crystal-clear:
- - name: CI + - name: Quick tests & benchmarks working-directory: v2 run: make test-quick
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/execution.yml(1 hunks).github/workflows/v2.yml(1 hunks)execution/Makefile(0 hunks)v2/Makefile(2 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- execution/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (1)
v2/Makefile (1)
3-3:-count=1flag addition is spot-on
Guarantees a fresh run every time and avoids puzzling cache-hits in CI.
Include benchmarks into quick tests of v2. In addition, I have attempted to simplify Makefile rules for testing. Instead of aliasing let's use "test" and "test-quick" rules directly from github workflows.
ysmolski
force-pushed
the
yury/eng-7616-engine-run-v2-benchmark-on-ci-to-detect-regression-early
branch
from
f5f6358 to
0354160
Compare
ysmolski
deleted the
yury/eng-7616-engine-run-v2-benchmark-on-ci-to-detect-regression-early
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.211](v2.0.0-rc.210...v2.0.0-rc.211) (2025-07-28) ### Features * redesign handling for lists in gRPC ([#1246](#1246)) ([a06c9db](a06c9db)) ### Bug Fixes * disable minifier for gRPC datasource ([#1249](#1249)) ([9a26e5c](9a26e5c)) * test v2 benchmarks on ci ([#1238](#1238)) ([d9cfb21](d9cfb21)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Redesigned handling of lists in the gRPC datasource. * **Bug Fixes** * Disabled the minifier for the gRPC datasource. * Enabled testing of version 2 benchmarks on continuous integration (CI). * **Documentation** * Added a changelog entry for version 2.0.0-rc.211. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Include benchmarks into quick tests of v2.
In addition, I have attempted to simplify Makefile rules for testing. Instead of aliasing let's use "test" and "test-quick" rules directly from github workflows.
Specifically changes in Makefile rules:
test-no-race was removed as duplicate of test-quick ,
ci and ci-full were removed, github workflow was made to use test and test-quick
Summary by CodeRabbit
Summary by CodeRabbit
Chores
Refactor
Checklist